-
-
Notifications
You must be signed in to change notification settings - Fork 487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add <LinkButton>
component
#1784
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
🦋 Changeset detectedLatest commit: 5ea0fc5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
.sl-markdown-content a:not(:where(:is(.not-content, .not-content *))) { | ||
color: var(--sl-color-text-accent); | ||
} | ||
.sl-markdown-content a:hover:not(:where(.not-content *)) { | ||
.sl-markdown-content a:hover:not(:where(:is(.not-content, .not-content *))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are used to avoid <LinkButton>
getting Markdown link styles applied to it by specifying the not-content
class on the link itself and not a wrapping element.
I'm wondering if this is a change that could be applied to some other places to avoid relying on a wrapping element to avoid Markdown styles 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d like to avoid this if we can to avoid introducing inconsistencies.
The main reason .not-content
applies to children not the element with the class is due to spacing. Consider an example like this:
<p>Lorem ipsum...</p>
<div class="not-content">Some non-content...</div>
<p>Dolor sic...</p>
If not-content
removed the margin styles from the <div>
in this example, it would end up squished next to the first paragraph.
I have sometimes considered whether we should have separate rules for escaping spacing vs other styles, but I’m not sure that’s particularly easy to understand or communicate.
What would be better would be to either give the Markdown styles a lower specificity (e.g. wrap each selector in :where()
maybe?) or move forward with the @layer
refactor. That would make it less painful for user components (or our own in this case) to escape the default styles without the extra not-content
class. (not-content
would still be needed for circumstances that benefit from a full reset, but there wouldn’t be the temptation to use it because otherwise !important
is needed to enforce a component style.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this makes sense, in this case this was a link color only change but there is no guarantee this would stay the same any way. I think in this case, we should be able to just specify the color
in the various selectors, e.g. .sl-link-button.primary
, .sl-link-button.secondary
, and .sl-link-button.minimal
.
Following the last Talking & Doc'ing session discussions, I updated the pull request to include the following changes:
|
* main: (57 commits) Add type checking job to the CI workflow (withastro#1827) [ci] format i18n(pt-BR): Update `components.mdx` (withastro#1815) [ci] format i18n(ru): update translations (withastro#1825) i18n(pt-BR): Update `css-and-tailwind.mdx` (withastro#1817) i18n(es): updates `pages` (withastro#1823) i18n(es): update `i18n` (withastro#1822) i18n(es): updates `overrides` (withastro#1820) i18n(es): update `guides/components` and add `syncKey` to various pages (withastro#1818) [ci] format i18n(es): update `community-content` (withastro#1824) i18n(es): update `configuration` (withastro#1821) i18n(es): update `frontmatter` (withastro#1819) i18n(fr): update `guides/pages.mdx` (withastro#1800) i18n(fr): update `reference/overrides.md` (withastro#1803) i18n(fr): update `reference/frontmatter.md` (withastro#1802) i18n(fr): update `reference/configuration.mdx` (withastro#1801) i18n(fr): update `guides/i18n.mdx` (withastro#1799) i18n(fr): update `guides/components` and add `syncKey` to various pages (withastro#1797) ...
@@ -51,10 +51,6 @@ | |||
"types": "./components/Sidebar.astro.tsx", | |||
"import": "./components/Sidebar.astro" | |||
}, | |||
"./components/CallToAction.astro": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May want to call this out in the changeset as well
One additional thing: Not sure if you were basing this off of the Figma file or not, but how about links without any text and only an icon? Is that something you wanted to support in this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for the PR @HiDeoo and apologies for the belated review — I left some comments!
how about links without any text and only an icon? Is that something you wanted to support in this?
Regarding this comment from @lorenzolewis, I think it’s OK not to support that for now. I’m not 100% sure what the intended usage was for icon buttons in Figma and they complicate things somewhat — icon-only buttons still need an accessible label etc. Also, might be possible to work around for someone who really needs it:
<LinkButton href="/astro" icon="astro">
<span class="sr-only">Astro</span>
</LinkButton>
.sl-markdown-content a:not(:where(:is(.not-content, .not-content *))) { | ||
color: var(--sl-color-text-accent); | ||
} | ||
.sl-markdown-content a:hover:not(:where(.not-content *)) { | ||
.sl-markdown-content a:hover:not(:where(:is(.not-content, .not-content *))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d like to avoid this if we can to avoid introducing inconsistencies.
The main reason .not-content
applies to children not the element with the class is due to spacing. Consider an example like this:
<p>Lorem ipsum...</p>
<div class="not-content">Some non-content...</div>
<p>Dolor sic...</p>
If not-content
removed the margin styles from the <div>
in this example, it would end up squished next to the first paragraph.
I have sometimes considered whether we should have separate rules for escaping spacing vs other styles, but I’m not sure that’s particularly easy to understand or communicate.
What would be better would be to either give the Markdown styles a lower specificity (e.g. wrap each selector in :where()
maybe?) or move forward with the @layer
refactor. That would make it less painful for user components (or our own in this case) to escape the default styles without the extra not-content
class. (not-content
would still be needed for circumstances that benefit from a full reset, but there wouldn’t be the temptation to use it because otherwise !important
is needed to enforce a component style.)
:global(.sl-markdown-content) .sl-link-button { | ||
margin-inline-end: 0.5em; | ||
} | ||
:global(.sl-markdown-content) .sl-link-button:not(:where(p *)) { | ||
margin-block: 1rem; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me a bit nervous, but I guess it’s OK 😅
I don’t love the context-specific styling, but I also thought about it a bit and can see that without this, the default behaviour would feel pretty broken. Other solutions I can think of are also annoying, so we should probably go with this.
|
||
<LinkButton href="/getting-started/">Get started</LinkButton> | ||
<LinkButton href="https://docs.astro.build" variant="secondary" icon="external"> | ||
Related: Astro |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting that this renders as <a ...><p>Related: Astro</p></a>
Not sure if there’s anything we could or should do about that? It’s not invalid HTML and I guess doesn’t really have any bad side effects.
* main: (275 commits) [ci] release (withastro#2165) Define well-known RTL locales before calling `getLocaleInfo()` for default locale (withastro#2167) Update dependencies (withastro#2166) Improve page load performance (withastro#2155) docs: Add CodeSweetly to showcase (withastro#2160) [ci] release (withastro#2145) Fix bug for projects with spaces in their pathname (withastro#2156) ci: update file icons (withastro#2157) [ci] format Merge <link rel="canonical" /> tags, quick fixes (withastro#2153) (withastro#2154) Add two new showcase sites (withastro#2149) [ci] format i18n(zh-cn): Update `environmental-impact.md` (withastro#2148) add Saasfly showcase (withastro#2147) docs: add 'og:image:alt' metadata (withastro#2143) Deleting unnecessary twitter meta tags, quick fixes (withastro#2137) [ci] format i18n(es): update `environmental-impact` (withastro#2144) [ci] release (withastro#2142) feat: Add Pinterest icon to social list (withastro#2135) ...
Co-authored-by: Chris Swithinbank <357379+delucis@users.noreply.github.com>
Co-authored-by: Chris Swithinbank <357379+delucis@users.noreply.github.com>
✅ Deploy Preview for astro-starlight ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Updated the branch and push a few changes that should address all the great feedback, thanks for the review 🙌 I also temporarily added back the demo page, it was useful to me to get back into the context of the PR so thought I might as well include it in case it can help and I'll remove it later (also unticked the associated todo in the PR body). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @HiDeoo — left a couple more comments now I had the preview page to play with, but I think it looks pretty close to being ready 🙌
Co-authored-by: Chris Swithinbank <357379+delucis@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we’re ready as far as I’m concerned — thank you!
Re: spacing on mobile vs desktop, I see what you mean, but I still think it will look OK in context. One of the slightly tricky things about the demo page is that there are buttons everywhere, but hopefully in reality people will only be using one or two 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I declare this <LinkButton>
seaworthy — anchors away! 🚢
What kind of changes does this PR include?
Description
This PR adds a new
<LinkButton>
component adressing the request in the built-in components discussion.Preview playground page: https://starlight-git-fork-hideoo-hd-feat-link-button-astrodotbuild.vercel.app/guides/components/#link-buttons
Visually, this is almost identical to the internal
<CallToAction>
component (now removed and replaced by the new component) that was used for actions in the hero component.The name is still up for discussion, some design system use
CTALink
orCallToAction
but this may not be obvious to everyone. Some others useButtonLink
but as pointed out by Chris, we already have a<LinkCard>
component so this uses the same pattern.Using something like
<Button>
may be confusing as this is a link that should be used for navigation and not a form primitive for example.Remaining tasks
docs/src/assets/landing.css
)docs/src/content/docs/reference/link-button.mdx
)